feat(connect): Add Connect module#1597
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughThis PR adds a complete Connect API client module to the WorkOS SDK with TypeScript interfaces, serializers, a Connect client class (CRUD, pagination, OAuth completion, client-secret management), JSON fixtures and Jest tests, and public SDK integration plus manifest updates. ChangesConnect API Client Implementation
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR introduces a new
Confidence Score: 3/5The new Connect module is mostly well-structured, but several issues in connect.ts and the update serializer produce wrong wire payloads that would silently affect real API calls. The auto-pagination path passes camelCase options to the API for pages beyond the first, so organizationId filtering silently breaks for multi-page results. The createOAuthApplication/createM2MApplication convenience methods build wire bodies by hand and diverge from the serializer behavior for null fields. The update serializer converts every absent optional field to explicit null, which would clear existing description, scopes, and redirect URIs on a REST PUT even when the caller only wanted to rename the application. src/connect/connect.ts (auto-pagination options and convenience method bodies), src/connect/serializers/update-oauth-application.serializer.ts (null-clearing on partial updates) Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/connect/connect.ts (1)
210-228: ⚡ Quick winManual body construction bypasses serialization logic.
Similar to
createOAuthApplication, this method manually builds the request body instead of usingserializeCreateM2MApplication(imported at line 51). While M2M applications don't have nested objects requiring serialization like OAuth applications do, this is still inconsistent with:
- The
createApplicationmethod (lines 139-163) which uses serializers- The
updateApplicationmethod (lines 257-269) which uses serializers♻️ Recommended refactor to use the serializer
async createM2MApplication( name: string, organizationId: string, description?: string | null, scopes?: string[] | null, ): Promise<ConnectApplication> { - const body: Record<string, unknown> = { - application_type: 'm2m', - name: name, - organization_id: organizationId, - }; - if (description !== undefined) body.description = description; - if (scopes !== undefined) body.scopes = scopes; - const { data } = await this.workos.post<ConnectApplicationResponse>( + const payload: CreateM2MApplication = { + applicationType: 'm2m', + name, + organizationId, + ...(description !== undefined && { description }), + ...(scopes !== undefined && { scopes }), + }; + const { data } = await this.workos.post<ConnectApplicationResponse, CreateM2MApplicationResponse>( '/connect/applications', - body, + serializeCreateM2MApplication(payload), ); return deserializeConnectApplication(data); }src/connect/serializers.spec.ts (1)
43-43: ⚡ Quick winType safety bypassed with
as anycast.The
as anycast defeats TypeScript's type checking. While this may be intentional in generated code to handle minor fixture/interface mismatches, it means type errors in serializers won't be caught by these tests.If oagen controls the generation, consider ensuring fixtures exactly match their corresponding interfaces to avoid needing
as any.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6438bd4-4c7e-48e1-b5b9-0e0b42b27e73
📒 Files selected for processing (49)
.oagen-manifest.jsonsrc/connect/connect.spec.tssrc/connect/connect.tssrc/connect/fixtures/application-credentials-list-item.jsonsrc/connect/fixtures/connect-application.jsonsrc/connect/fixtures/create-application-secret.jsonsrc/connect/fixtures/create-m2m-application.jsonsrc/connect/fixtures/create-oauth-application.jsonsrc/connect/fixtures/external-auth-complete-response.jsonsrc/connect/fixtures/list-connect-application.jsonsrc/connect/fixtures/new-connect-application-secret.jsonsrc/connect/fixtures/redirect-uri-input.jsonsrc/connect/fixtures/update-oauth-application.jsonsrc/connect/fixtures/user-consent-option-choice.jsonsrc/connect/fixtures/user-consent-option.jsonsrc/connect/fixtures/user-management-login-request.jsonsrc/connect/fixtures/user-object.jsonsrc/connect/interfaces/application-credentials-list-item.interface.tssrc/connect/interfaces/connect-application.interface.tssrc/connect/interfaces/create-application-secret.interface.tssrc/connect/interfaces/create-m2m-application.interface.tssrc/connect/interfaces/create-oauth-application.interface.tssrc/connect/interfaces/external-auth-complete-response.interface.tssrc/connect/interfaces/index.tssrc/connect/interfaces/list-applications-options.interface.tssrc/connect/interfaces/new-connect-application-secret.interface.tssrc/connect/interfaces/redirect-uri-input.interface.tssrc/connect/interfaces/update-oauth-application.interface.tssrc/connect/interfaces/user-consent-option-choice.interface.tssrc/connect/interfaces/user-consent-option.interface.tssrc/connect/interfaces/user-management-login-request.interface.tssrc/connect/interfaces/user-object.interface.tssrc/connect/serializers.spec.tssrc/connect/serializers/application-credentials-list-item.serializer.tssrc/connect/serializers/connect-application.serializer.tssrc/connect/serializers/create-application-secret.serializer.tssrc/connect/serializers/create-m2m-application.serializer.tssrc/connect/serializers/create-oauth-application.serializer.tssrc/connect/serializers/external-auth-complete-response.serializer.tssrc/connect/serializers/index.tssrc/connect/serializers/new-connect-application-secret.serializer.tssrc/connect/serializers/redirect-uri-input.serializer.tssrc/connect/serializers/update-oauth-application.serializer.tssrc/connect/serializers/user-consent-option-choice.serializer.tssrc/connect/serializers/user-consent-option.serializer.tssrc/connect/serializers/user-management-login-request.serializer.tssrc/connect/serializers/user-object.serializer.tssrc/index.tssrc/workos.ts
| /** An ISO 8601 timestamp. */ | ||
| updatedAt: Date; | ||
| /** The type of the application. */ | ||
| applicationType?: 'm2m'; |
There was a problem hiding this comment.
The CreateM2MApplication and CreateOAuthApplication types look good, but this ConnectApplication type is a bit weird. I'd expect applicationType to be a discriminator between the two as the form a kind of union, but here it only lists m2m.
Seems like this modeling isn't quite right. Is there a way to get the generator to give us something better?
| * @throws {UnprocessableEntityException} 422 | ||
| */ | ||
| async createApplication( | ||
| payload: CreateOAuthApplication | CreateM2MApplication, |
There was a problem hiding this comment.
Nice, this type of union of the different applications is more what I was imagining. 💯
mthadley
left a comment
mthadley
left a comment
There was a problem hiding this comment.
@gjtorikian Overall, looks solid.
The only thing that looks off and that could be usability problem is this, so requesting changes until it can be addressed.
| export const serializeUpdateOAuthApplication = ( | ||
| model: UpdateOAuthApplication, | ||
| ): UpdateOAuthApplicationResponse => ({ | ||
| name: model.name, | ||
| description: model.description ?? null, | ||
| scopes: model.scopes ?? null, | ||
| redirect_uris: | ||
| model.redirectUris != null | ||
| ? model.redirectUris.map(serializeRedirectUriInput) | ||
| : null, | ||
| }); |
There was a problem hiding this comment.
description, scopes, and redirect_uris are serialized with ?? null or a falsy-null fallback, so when a caller passes only { name: 'New Name' }, the wire body becomes { name: "New Name", description: null, scopes: null, redirect_uris: null }. A typical REST API that receives explicit null on a PUT treats it as "clear this field", meaning existing description, scopes, and redirect URIs would be erased even though the caller never touched them.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/connect/connect.ts (1)
176-200:⚠️ Potential issue | 🔴 Critical | ⚖️ Poor tradeoffCritical: Still manually constructing request body instead of using serializer.
This method manually builds the wire-format request body (lines 185-194) with snake_case keys (
application_type,is_first_party,redirect_uris) instead of usingserializeCreateOAuthApplication, which is imported at line 50. This was flagged in a previous review as addressed in commits c82e4e8 to 4c21b9c, but the manual construction is still present in the current code.The manual approach has a critical flaw: line 192 assigns
redirectUrisdirectly without callingserializeRedirectUriInputon each item, which transforms thedefaultfield fromundefinedtonullper the wire format contract.This breaks consistency with:
createApplication(line 150) — usesserializeCreateOAuthApplicationupdateApplication(line 266) — usesserializeUpdateOAuthApplication🔧 Refactor to use the serializer
async createOAuthApplication( name: string, isFirstParty: boolean, description?: string | null, scopes?: string[] | null, redirectUris?: RedirectUriInput[] | null, usesPkce?: boolean | null, organizationId?: string | null, ): Promise<ConnectApplication> { - const body: Record<string, unknown> = { - application_type: 'oauth', - name: name, - is_first_party: isFirstParty, - }; - if (description !== undefined) body.description = description; - if (scopes !== undefined) body.scopes = scopes; - if (redirectUris !== undefined) body.redirect_uris = redirectUris; - if (usesPkce !== undefined) body.uses_pkce = usesPkce; - if (organizationId !== undefined) body.organization_id = organizationId; - const { data } = await this.workos.post<ConnectApplicationResponse>( + const payload: CreateOAuthApplication = { + applicationType: 'oauth', + name, + isFirstParty, + ...(description !== undefined && { description }), + ...(scopes !== undefined && { scopes }), + ...(redirectUris !== undefined && { redirectUris }), + ...(usesPkce !== undefined && { usesPkce }), + ...(organizationId !== undefined && { organizationId }), + }; + const { data } = await this.workos.post< + ConnectApplicationResponse, + CreateOAuthApplicationResponse + >( '/connect/applications', - body, + serializeCreateOAuthApplication(payload), ); return deserializeConnectApplication(data); }
🧹 Nitpick comments (4)
src/connect/fixtures/external-auth-complete-response.json (1)
2-2: ⚡ Quick winUse a clearly non-secret
statefixture value.This JWT-like sample triggers secret scanners and creates unnecessary security noise in CI. Prefer an obvious test placeholder (e.g.,
state=test_state_value).♻️ Suggested fixture update
- "redirect_uri": "https://your-authkit-domain.workos.com/oauth/authorize/complete?state=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdGF0ZSI6InJhbmRvbV9zdGF0ZV9zdHJpbmciLCJpYXQiOjE3NDI2MDQ4NTN9.abc123def456ghi789" + "redirect_uri": "https://your-authkit-domain.workos.com/oauth/authorize/complete?state=test_state_value"src/connect/fixtures/new-connect-application-secret.json (1)
8-8: ⚡ Quick winReplace the secret-looking literal with an explicit test sentinel.
This value looks like a real credential and can trip leak detection tooling. Use an obviously fake fixture value (e.g.,
not_a_real_secret_for_tests).♻️ Suggested fixture update
- "secret": "abc123def456ghi789jkl012mno345pqr678stu901vwx234yz" + "secret": "not_a_real_secret_for_tests"src/connect/connect.spec.ts (2)
54-56: ⚡ Quick winAssert against fixture fields instead of duplicating sensitive-looking literals.
These hardcoded values duplicate fixture content and increase false-positive leak noise. Assert against imported fixture properties to keep a single source of truth.
♻️ Suggested test update
- expect(result.redirectUri).toBe( - 'https://your-authkit-domain.workos.com/oauth/authorize/complete?state=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdGF0ZSI6InJhbmRvbV9zdGF0ZV9zdHJpbmciLCJpYXQiOjE3NDI2MDQ4NTN9.abc123def456ghi789', - ); + expect(result.redirectUri).toBe( + externalAuthCompleteResponseFixture.redirect_uri, + ); ... - expect(result.secret).toBe( - 'abc123def456ghi789jkl012mno345pqr678stu901vwx234yz', - ); + expect(result.secret).toBe(newConnectApplicationSecretFixture.secret);Also applies to: 191-193
71-74: ⚡ Quick winTighten
listApplicationsassertions to validate deserialization, not just shape.This test currently verifies non-empty output but not field mapping. Reuse the helper on the first item and assert
listMetadatavalues from fixture to catch serializer regressions.♻️ Suggested test update
expect(fetchSearchParams()).toHaveProperty('order'); expect(Array.isArray(data)).toBe(true); - expect(listMetadata).toBeDefined(); expect(data.length).toBeGreaterThan(0); + expectConnectApplicationM2M(data[0]); + expect(listMetadata).toEqual({ before: null, after: null });
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f2b581e-0f09-4e0b-975a-5346dbe38cf5
📒 Files selected for processing (50)
.oagen-manifest.jsonsrc/connect/connect.spec.tssrc/connect/connect.tssrc/connect/fixtures/application-credentials-list-item.jsonsrc/connect/fixtures/connect-application.jsonsrc/connect/fixtures/create-application-secret.jsonsrc/connect/fixtures/create-m2m-application.jsonsrc/connect/fixtures/create-oauth-application.jsonsrc/connect/fixtures/external-auth-complete-response.jsonsrc/connect/fixtures/list-connect-application.jsonsrc/connect/fixtures/new-connect-application-secret.jsonsrc/connect/fixtures/redirect-uri-input.jsonsrc/connect/fixtures/update-oauth-application.jsonsrc/connect/fixtures/user-consent-option-choice.jsonsrc/connect/fixtures/user-consent-option.jsonsrc/connect/fixtures/user-management-login-request.jsonsrc/connect/fixtures/user-object.jsonsrc/connect/interfaces/application-credentials-list-item.interface.tssrc/connect/interfaces/connect-application-redirect-uri.interface.tssrc/connect/interfaces/connect-application.interface.tssrc/connect/interfaces/create-application-secret.interface.tssrc/connect/interfaces/create-m2m-application.interface.tssrc/connect/interfaces/create-oauth-application.interface.tssrc/connect/interfaces/external-auth-complete-response.interface.tssrc/connect/interfaces/index.tssrc/connect/interfaces/list-applications-options.interface.tssrc/connect/interfaces/new-connect-application-secret.interface.tssrc/connect/interfaces/redirect-uri-input.interface.tssrc/connect/interfaces/update-oauth-application.interface.tssrc/connect/interfaces/user-consent-option-choice.interface.tssrc/connect/interfaces/user-consent-option.interface.tssrc/connect/interfaces/user-management-login-request.interface.tssrc/connect/interfaces/user-object.interface.tssrc/connect/serializers/application-credentials-list-item.serializer.tssrc/connect/serializers/connect-application-redirect-uri.serializer.tssrc/connect/serializers/connect-application.serializer.tssrc/connect/serializers/create-application-secret.serializer.tssrc/connect/serializers/create-m2m-application.serializer.tssrc/connect/serializers/create-oauth-application.serializer.tssrc/connect/serializers/external-auth-complete-response.serializer.tssrc/connect/serializers/index.tssrc/connect/serializers/new-connect-application-secret.serializer.tssrc/connect/serializers/redirect-uri-input.serializer.tssrc/connect/serializers/update-oauth-application.serializer.tssrc/connect/serializers/user-consent-option-choice.serializer.tssrc/connect/serializers/user-consent-option.serializer.tssrc/connect/serializers/user-management-login-request.serializer.tssrc/connect/serializers/user-object.serializer.tssrc/index.tssrc/workos.ts
✅ Files skipped from review due to trivial changes (32)
- src/connect/fixtures/create-m2m-application.json
- src/connect/serializers/create-m2m-application.serializer.ts
- src/connect/fixtures/create-oauth-application.json
- src/connect/serializers/application-credentials-list-item.serializer.ts
- src/connect/fixtures/user-object.json
- src/connect/fixtures/update-oauth-application.json
- src/connect/fixtures/redirect-uri-input.json
- src/connect/interfaces/list-applications-options.interface.ts
- src/connect/interfaces/connect-application-redirect-uri.interface.ts
- src/connect/fixtures/user-consent-option-choice.json
- src/connect/interfaces/create-m2m-application.interface.ts
- src/connect/fixtures/application-credentials-list-item.json
- src/connect/serializers/create-oauth-application.serializer.ts
- src/connect/fixtures/create-application-secret.json
- src/connect/serializers/user-management-login-request.serializer.ts
- src/connect/interfaces/user-consent-option.interface.ts
- src/connect/serializers/new-connect-application-secret.serializer.ts
- src/connect/interfaces/user-consent-option-choice.interface.ts
- src/connect/interfaces/user-object.interface.ts
- src/connect/interfaces/update-oauth-application.interface.ts
- src/connect/interfaces/redirect-uri-input.interface.ts
- src/connect/serializers/connect-application.serializer.ts
- src/connect/serializers/user-consent-option-choice.serializer.ts
- src/connect/interfaces/user-management-login-request.interface.ts
- src/connect/serializers/user-consent-option.serializer.ts
- src/connect/interfaces/connect-application.interface.ts
- src/connect/interfaces/external-auth-complete-response.interface.ts
- src/connect/serializers/external-auth-complete-response.serializer.ts
- .oagen-manifest.json
- src/connect/fixtures/user-management-login-request.json
- src/connect/interfaces/application-credentials-list-item.interface.ts
- src/connect/interfaces/create-application-secret.interface.ts
| async createM2MApplication( | ||
| name: string, | ||
| organizationId: string, | ||
| description?: string | null, | ||
| scopes?: string[] | null, | ||
| ): Promise<ConnectApplication> { | ||
| const body: Record<string, unknown> = { | ||
| application_type: 'm2m', | ||
| name: name, | ||
| organization_id: organizationId, | ||
| }; | ||
| if (description !== undefined) body.description = description; | ||
| if (scopes !== undefined) body.scopes = scopes; | ||
| const { data } = await this.workos.post<ConnectApplicationResponse>( | ||
| '/connect/applications', | ||
| body, | ||
| ); | ||
| return deserializeConnectApplication(data); | ||
| } |
There was a problem hiding this comment.
Critical: Manually constructing request body instead of using serializer.
This method manually builds the wire-format request body (lines 216-222) instead of using serializeCreateM2MApplication (imported at line 51). This is the same critical issue present in createOAuthApplication and breaks consistency with createApplication (line 152), which correctly uses the serializer when applicationType is 'm2m'.
🔧 Refactor to use the serializer
async createM2MApplication(
name: string,
organizationId: string,
description?: string | null,
scopes?: string[] | null,
): Promise<ConnectApplication> {
- const body: Record<string, unknown> = {
- application_type: 'm2m',
- name: name,
- organization_id: organizationId,
- };
- if (description !== undefined) body.description = description;
- if (scopes !== undefined) body.scopes = scopes;
- const { data } = await this.workos.post<ConnectApplicationResponse>(
+ const payload: CreateM2MApplication = {
+ applicationType: 'm2m',
+ name,
+ organizationId,
+ ...(description !== undefined && { description }),
+ ...(scopes !== undefined && { scopes }),
+ };
+ const { data } = await this.workos.post<
+ ConnectApplicationResponse,
+ CreateM2MApplicationResponse
+ >(
'/connect/applications',
- body,
+ serializeCreateM2MApplication(payload),
);
return deserializeConnectApplication(data);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async createM2MApplication( | |
| name: string, | |
| organizationId: string, | |
| description?: string | null, | |
| scopes?: string[] | null, | |
| ): Promise<ConnectApplication> { | |
| const body: Record<string, unknown> = { | |
| application_type: 'm2m', | |
| name: name, | |
| organization_id: organizationId, | |
| }; | |
| if (description !== undefined) body.description = description; | |
| if (scopes !== undefined) body.scopes = scopes; | |
| const { data } = await this.workos.post<ConnectApplicationResponse>( | |
| '/connect/applications', | |
| body, | |
| ); | |
| return deserializeConnectApplication(data); | |
| } | |
| async createM2MApplication( | |
| name: string, | |
| organizationId: string, | |
| description?: string | null, | |
| scopes?: string[] | null, | |
| ): Promise<ConnectApplication> { | |
| const payload: CreateM2MApplication = { | |
| applicationType: 'm2m', | |
| name, | |
| organizationId, | |
| ...(description !== undefined && { description }), | |
| ...(scopes !== undefined && { scopes }), | |
| }; | |
| const { data } = await this.workos.post< | |
| ConnectApplicationResponse, | |
| CreateM2MApplicationResponse | |
| >( | |
| '/connect/applications', | |
| serializeCreateM2MApplication(payload), | |
| ); | |
| return deserializeConnectApplication(data); | |
| } |
| createdAt: Date; | ||
| /** An ISO 8601 timestamp. */ | ||
| updatedAt: Date; | ||
| applicationType: 'oauth'; |
There was a problem hiding this comment.
hmm, looks like we are missing a "description" somewhere for applicationType. Maybe something to track down later.
There was a problem hiding this comment.
good catch. another bug around the discriminated union -- will fix
2 participants
Summary
Connectresource client onWorkOSfor managing OAuth and M2M applications, application secrets, redirect URIs, user consent options, and the external auth login/consent flow.src/connect/from the latest oagen spec.src/connect/interfacesfrom the package barrel so consumers can import the new types.Test plan
npm testpasses (unit + serializer specs)npm run buildsucceeds with the new exportsworkos.connectis reachable on the client and methods type-check from a consumerSummary by CodeRabbit
New Features
Tests